Fix processing instructions gaining an extra "?" when an HTML-parsed document is serialized by jakejackson1 · Pull Request #68 · GravityPDF/querypath

jakejackson1 · 2026-08-21T04:36:44Z

Fixes #65

The bug

A processing instruction in an HTML-parsed document gained an extra ? every time the document was serialized:

$qp = htmlqp('<html><body><h1><?php echo $title; ?></h1></body></html>');
$qp->top()->find('body')->innerHTML();
// was:  <h1><?php echo $title; ??></h1>
// now:  <h1><?php echo $title; ?></h1>

Round-trip twice and you got ???>, and so on — the output was no longer valid PHP.

Root cause

libxml's HTML parser stores the closing ? of a processing instruction as part of the node's data; its XML parser and the Masterminds HTML5 parser do not.

$d = new DOMDocument();
$d->loadHTML('<html><body><h1><?php echo $t; ?></h1></body></html>');
// $pi->data === 'echo $t; ?'   <-- trailing "?" retained

$d2 = new DOMDocument();
$d2->loadXML('<root><?php echo $t; ?></root>');
// $pi->data === 'echo $t; '    <-- no trailing "?"

Anything that appends its own ?> then doubles it up. That is saveXML() (used by html() on a non-root node, innerHTML(), innerXML(), innerXHTML(), xml(), writeXML()) and the Masterminds serializer (html5(), innerHTML5(), writeHTML5()).

One correction to the issue's analysis, which changed the shape of the fix: saveHTML() does not compensate. libxml's HTML serializer writes a processing instruction verbatim as <?target data> and never adds a ? of its own — writeHTML() only looked correct because the parser had left one in the data. Confirmed on PHP 8.3.16 / libxml 2.9.13:

$pi->data = 'echo $t; ';          // strip the retained "?"
$d->saveHTML($body);              // <body><h1><?php echo $t; ></h1></body>   <-- broken

The corollary is that writeHTML() on an XML-parsed document was already broken in the same way, emitting <?php echo $t; >.

Approach: normalise on load

I went with the reporter's second suggestion — strip one trailing ? from processing instruction data whenever the libxml HTML parser is used (DOM::normalizeProcessingInstructions(), reached from DOM::loadHTMLString() and DOM::loadHTMLFile(), which are the only two ways QueryPath drives libxml's HTML parser).

The scan for processing instructions is skipped unless the source contains the literal ?>. Data can only end in ? if a ? sat immediately before the closing >, so this drops no real match, and it keeps load cost at parity with main for the documents — almost all of them — that have no processing instructions to normalise.

This gives every parser QueryPath supports a single invariant — processing instruction data never contains the closing ? — which fixes all of the affected serializers at once, matches what the XML and HTML5 parsers already produce, and fixes the read side: $pi->data now hands back usable PHP source instead of source with a stray ? glued on.

Exactly one ? is stripped, so a processing instruction whose content legitimately ends in ? (<?php $a = 1; ??>) still round-trips. XML-parsed documents and html5qp() documents are not touched, since only the libxml HTML paths call the normaliser.

Because libxml's HTML serializer does not add the terminator back, the two saveHTML()-based output paths — writeHTML() and the whole-document branch of html() — now go through DOMQuery::saveDocumentHTML(), which re-appends the ? for the duration of the write and removes it again in a finally. There is a test asserting the document is unchanged afterwards.

Documents QueryPath did not parse

The invariant can only be established for a document QueryPath parses itself, so it is only paid back for one. Whether an already-parsed document handed to the constructor — a DOMDocument, DOMNode, SimpleXMLElement or node list — holds to it cannot be determined after the fact:

'<?php $a = 1; ??>'   // XML parser  -> data '$a = 1; ?'    (ends in "?")
                      // HTML parser -> data '$a = 1; ??'   (ends in "??")

Both end in ?, and nothing in the tree records which parser produced them. Stripping would corrupt the first; re-appending in the serializer would corrupt the second. Normalising a second time is not an option either — it would strip a ? that legitimately belongs to the content, eroding <?php $a = 1; ??> a character per pass.

So the fact is recorded rather than guessed at, and it is recorded on the document: documents QueryPath parses are QueryPath\Document, a DOMDocument subclass that carries no state of its own. The type is the marker. saveDocumentHTML() compensates when it sees one and writes anything else as-is, which is what main already did for caller-supplied documents.

Recording it on the document rather than on the DOMQuery matters, because a document outlives the query object that parsed it. Every route to a second DOMQuery over one document reaches the same document and therefore the same answer — iteration, add(), remove(), replaceAll(), branch(), QueryPath::with(), and the bundled extensions. A boolean on the query object would have to be hand-copied at each of those sites, and any new one would silently lose it.

One subtlety worth flagging for review: DOM::createDocument() calls registerNodeClass(DOMDocument::class, Document::class), and the fix does not work without it. PHP rebuilds a document's wrapper object whenever it is reached through $node->ownerDocument after the original wrapper has been released, and rebuilds it as a plain DOMDocument — discarding the one fact the type exists to record. With the registration the type survives; there is a test for it.

Rejected alternative: `saveHTML($node)` in the HTML-oriented serializers

The narrower option was to swap saveXML($node) for saveHTML($node) in html()/innerHTML(). I measured the difference on an HTML-parsed document and it is far too large to be a bug fix:

	`saveXML($node)` (today)	`saveHTML($node)`
void elements	`<br/>`, `<hr/>`, `<img …/>`	`<br>`, `<hr>`, `<img …>`
boolean attributes	`checked="checked"`	`checked`
empty elements	`<span/>`	`<span></span>`
`<script>` contents	wrapped in `<![CDATA[…]]>`	raw

That would break every caller relying on the current XHTML-ish output, and would not have covered innerHTML5()/html5() (Masterminds serializer) or the read side at all.

Behaviour changes

DOMProcessingInstruction::$data no longer carries a trailing ? for documents read via htmlqp(), qp() on an .html/.htm file, or use_parser => 'html'. Code that trimmed the ? itself with rtrim($pi->data, '?') is unaffected; code that used substr($data, 0, -1) unconditionally would now cut a real character.
writeHTML() on an XML-parsed document now emits <?php … ?> instead of <?php … >. This was a latent bug, fixed as a side effect.
An HTML processing instruction with no ? before the > (<?foo bar>) is serialized by writeHTML() as <?foo bar?> rather than <?foo bar>. Data is unchanged (nothing to strip); only the HTML write path now terminates it consistently with every other serializer.
Documents with no processing instructions serialize byte-for-byte as before; the normaliser and the write-path helper are both no-ops for them.

Tests

tests/Issues/Issue65Test.php (32 tests, 55 assertions), plus a tests/processing-instruction.html fixture to exercise the loadHTMLFile() path. tests/Issues/ is picked up by the existing recursive <directory>./tests/</directory> suite config, so no phpunit.xml change was needed.

Coverage:

the full reported surface — html() (node and whole-document), innerHTML(), innerXML(), innerXHTML(), innerHTML5(), html5(), xml()
qp() on a .html file, i.e. loadHTMLFile() as well as loadHTML()
three successive round trips are stable, with an explicit assertion that ??> never appears
the previously-working paths still work: writeHTML() (stdout and to a file), writeHTML5(), html5qp(), qp() in XML mode
writeHTML() leaves the document unchanged afterwards
<?php $a = 1; ??> is not double-stripped
<?foo bar> (no terminator) has nothing stripped
a document with no processing instructions serializes unchanged
a caller-supplied DOMDocument/DOMNode is serialized as-is, so the compensation cannot fire on a document that never had the ? stripped
branch(), and a DOMQuery constructed from another DOMQuery, both keep serializing correctly
every other route to a second query over one document: iterating a match set, qp() on a node and on the document, and remove() (which runs the legacy selector engine)
the document keeps its type when reached through ownerDocument after the original wrapper is gone
<?php $a = 1; ??> survives re-use — branch() and a second query over the same document do not erode it
writeHTML() on an XML-parsed document emits the terminator it used to drop

16 of the 32 fail on main and all pass with the fix.

capture() — the output-buffering helper these tests need, because phpunit.xml sets beStrictAboutOutputDuringTests — is on QueryPathTests\TestCase rather than this file, since the suite already open-codes that dance in eight places.

Verification

vendor/bin/phpunit — 387 tests, 1176 assertions, 2 pre-existing skips (create_function removed in PHP 8), 0 failures
composer run lint — clean
composer run lint:min-php — clean (PHPCompatibility, testVersion 7.1-)
composer run test:examples — all 17 examples pass

Verified locally on PHP 8.3.16 / libxml 2.9.13; CI covers 7.1–8.5.

Out of scope

html5qp() is unaffected by the doubling this PR fixes, but it still drops the terminator through html() and writeHTML() — <?php echo $title; >. That predates this change and reproduces on main and on 4.1.0, so it is tracked separately in #86 rather than folded in here; the fix for it needs a way to give a Masterminds-parsed document a doctype, which is a behaviour change of its own.

🤖 Generated with Claude Code

codecov · 2026-08-21T04:37:40Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.11%. Comparing base (03eaa7c) to head (807384f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main      #68      +/-   ##
============================================
+ Coverage     89.74%   90.11%   +0.37%     
- Complexity     1407     1421      +14     
============================================
  Files            26       26              
  Lines          3170     3198      +28     
============================================
+ Hits           2845     2882      +37     
+ Misses          325      316       -9

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

libxml's HTML parser stores the closing "?" of a `<?php ?>` block as part of the processing instruction's data, while its XML parser and the Masterminds HTML5 parser do not. Every serializer that appends its own "?>" therefore doubled it up, so `<?php echo $title; ?>` came back out of html(), innerHTML(), innerXML(), innerXHTML(), xml(), html5(), innerHTML5(), and writeXML() as `<?php echo $title; ??>`, gaining another "?" on every round trip. Reading $pi->data directly handed back source with a stray "?" glued to the end. QueryPath now strips it on load, giving every parser it supports the same invariant: processing instruction data never carries the closing "?". Exactly one "?" is removed, so a block whose content legitimately ends in one, `<?php $a = 1; ??>`, still round trips. The scan is skipped when the source contains no "?>" at all, which is almost every document. libxml's HTML serializer is then the one output path that needs the terminator back, since it writes a processing instruction verbatim and never appends one itself. saveDocumentHTML() restores it for the duration of the write and takes it off again afterwards, driving both passes from a single XPath walk. Knowing whether a document holds to the invariant cannot be worked out by inspecting it: the XML parser reading `<?php $a = 1; ??>` leaves exactly the trailing "?" that the HTML parser leaves for `<?php $a = 1; ?>`. Nor can it be re-established by normalising a second time, which would strip a "?" that belongs to the content. It is recorded instead by the document's type - documents QueryPath parses are QueryPath\Document - so it travels with the document rather than with the query object, and every route to a second DOMQuery over one document serializes correctly: iteration, add(), remove(), replaceAll(), branch(), QueryPath::with(), and the bundled extensions. registerNodeClass() keeps that type in place, because PHP otherwise rebuilds a document's wrapper as a plain DOMDocument once the original wrapper has been released. A DOMDocument supplied by the caller stays a plain DOMDocument, makes no such promise, and is written exactly as it was handed over. Note that html5qp() is unaffected by the doubling but still loses the terminator through html() and writeHTML(), which predates this change and is tracked separately in #86. Also adds TestCase::capture() for the many QueryPath methods that print rather than return, and folds the eight hand-rolled output-buffering blocks in DOMQueryTest onto it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

jakejackson1 force-pushed the issue-65 branch from 329474c to 43cd1e4 Compare August 24, 2026 02:37

jakejackson1 mentioned this pull request Aug 24, 2026

html5qp() documents lose the closing "?" of a PHP block when serialized with html()/writeHTML() #86

Open

jakejackson1 force-pushed the issue-65 branch from d3bf54e to b6c4a31 Compare August 24, 2026 02:56

jakejackson1 marked this pull request as ready for review August 24, 2026 03:04

jakejackson1 force-pushed the issue-65 branch from b6c4a31 to 807384f Compare August 24, 2026 04:49

jakejackson1 merged commit f38bb68 into main Aug 24, 2026
15 checks passed

jakejackson1 deleted the issue-65 branch August 24, 2026 04:51

Navigation Menu

Uh oh!

Fix processing instructions gaining an extra "?" when an HTML-parsed document is serialized - #68

Fix processing instructions gaining an extra "?" when an HTML-parsed document is serialized#68
jakejackson1 merged 1 commit into
mainfrom
issue-65

jakejackson1 commented Aug 21, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Aug 21, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

jakejackson1 commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug

Root cause

Approach: normalise on load

Documents QueryPath did not parse

Rejected alternative: saveHTML($node) in the HTML-oriented serializers

Behaviour changes

Tests

Verification

Out of scope

Uh oh!

codecov Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jakejackson1 commented Aug 21, 2026 •

edited

Loading

Rejected alternative: `saveHTML($node)` in the HTML-oriented serializers

codecov Bot commented Aug 21, 2026 •

edited

Loading